GCS_Common: NACK a failed partial mission write #10108
Conversation
|
I accept full responsibility :-) |
|
FWIW I would remove the STATUSTEXT in this instance because there is no context in which this information is useful to another GCS or system. |
That's my preference as well, but wasn't confident if any current GCS's are using that since we weren't nack'ing before. |
|
They'll be displaying it because it is a broadcast, but I don't see how they can use it. They'd have to be tracking the mission messages that aren't addressed to them to know what failed. ie irrelevant. Possibly a bit of a hole in the protocol though that there is no broadcast notification to other systems that mission upload has completed and they might want to re-sync to the version on the vehicle. |
WickedShell
force-pushed
the
wickedshell/mission-write-partial-nack
branch
from
a462a05
to
3eb4d36
Compare
|
I like the change, and it looks technically good. However, I'm not sure we should remove the text message at this point. Assuming no GCS out there is looking for the return mission message, we'll be removing any indication the user has that something has gone wrong. Also - shouldn't we really send back an ack for success if we're going to send a nak for failure? |
An ack is the vehicle asking you send it a mission item. (As far as I can tell anyways, we don't do any other kind of acking when you start a normal upload).
Yeah I'm willing to go with the majority view here, partial list hasn't been used by many people to the best of my knowledge which is part of why I was happy to just remove it. I haven't looked at the exsisting open impl's to see what they would do with the NACK. (Some might actually kill it correctly with this, others wouldn't) |
That is correct - or at least matches what is done for a normal upload. @peterbarker FYI I am in the process of documenting this - rendered here - @WickedShell is reviewing.
Good point. I'm changing my opinion to "leave the message in place too". The spec doesn't care - and this can be removed at some point in future. |
There was a problem hiding this comment.
@hamishwillee DEMANDS that the text message return. Or so I infer ;-)
:-) Yes, I am writing much more of the MAVLink spec in terms of must vs should. It does sound demanding, but otherwise people assume that whatever they will do is fine. |
There was a problem hiding this comment.
To be fair, on normal mission upload I don't see us sending any text message when an error occurs.
On the other hand, it is true that there was no NACK in this case so GCS could only rely on the text message to know if the error occurred - I doubt any did though: getting to this error would, in 99% of cases, mean the GCS implementation was wrong, so either the implementation has been fixed and this only matter for 1% of cases or the implementation is wrong and that's their fault 😛
WickedShell
force-pushed
the
wickedshell/mission-write-partial-nack
branch
from
3eb4d36
to
5fc975e
Compare
|
Statustext is back as per dev call, tagged it with a hard date after which I can nuke it. |
|
On Tue, 22 Jan 2019, WickedShell wrote:
Statustext is back as per dev call, tagged it with a hard date after which I can nuke it.
Not if I get to it first :-P
|
We should actually send an inband NACK rather then statustext for bad uploads. I left the status text in, but I'd actually like to see it come out. This is all @hamishwillee's fault ( mavlink/mavlink#1043 (comment) )